Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ffigen] Clean up global_test and comment out the flaky line. #1674

Merged
merged 13 commits into from
Jan 7, 2025
Merged

[ffigen] Clean up global_test and comment out the flaky line. #1674

merged 13 commits into from
Jan 7, 2025
+37 −87

Conversation

liamappelbe
Copy link
Contributor

@liamappelbe liamappelbe commented Oct 23, 2024 •
edited
Loading

global_test and global_native_test are exactly the same, except that one uses ordinary bindings, and the other uses @Native style bindings. They're flaky and I don't know why.

Here's what I know so far:

  • Occasionally they will both fail. It's always the case that both fail or both pass.
  • It's not that they interfere with each other. If I remove one of them from the suite, the other is still flaky.
  • It seems that the build itself is flaky. If I change the workflow to build once and run the tests 10 times, then it's always the case that there are either 0 or 10 failures
  • In case the built artifacts were conflicting, I changed the tests to use the same native code. They still have their own ffigen bindings, but the underlying so file only has one copy of the global variables. That didn't fix the flake, but it's still a good clean up, so I'll leave it.
  • Tried simplifying the ref counting test, but it's still flaky.
  • For now I've just commented out the flaky expectation, so I don't have to keep manually rerunning the flaky tests on unrelated PRs.

@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Oct 23, 2024
@liamappelbe liamappelbe mentioned this pull request Oct 23, 2024
Open
@liamappelbe liamappelbe changed the title Still debugging global flake [WIP] Still debugging global flake Oct 24, 2024
@coveralls
Copy link

coveralls commented Dec 18, 2024 •
edited
Loading

Coverage Status

coverage: 87.909% (-0.05%) from 87.958%
when pulling 4b61c6c on more_global_flake
into f2a0e28 on main.

@github-actions github-actions bot removed the type-infra A repository infrastructure change or enhancement label Dec 18, 2024
@liamappelbe liamappelbe changed the title [WIP] Still debugging global flake [ffigen] Fix global_test flake (finally!) Dec 18, 2024
@liamappelbe liamappelbe marked this pull request as ready for review December 18, 2024 04:54
@liamappelbe liamappelbe requested a review from dcharkes December 18, 2024 04:54
@liamappelbe
Copy link
Contributor Author

Of course, as soon as I put this PR out for review, it starts flaking again 😩

@liamappelbe liamappelbe removed the request for review from dcharkes December 18, 2024 05:12
@liamappelbe liamappelbe marked this pull request as draft December 18, 2024 05:12
@liamappelbe liamappelbe changed the title [ffigen] Fix global_test flake (finally!) WIP: [ffigen] Trying to fix global_test flake Dec 18, 2024
@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Dec 19, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 5, 2025 •
edited
Loading

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️ 
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@github-actions github-actions bot removed the type-infra A repository infrastructure change or enhancement label Jan 6, 2025
@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Jan 6, 2025
@github-actions github-actions bot removed the type-infra A repository infrastructure change or enhancement label Jan 7, 2025
@liamappelbe liamappelbe changed the title WIP: [ffigen] Trying to fix global_test flake [ffigen] Clean up global_test and comment out the flaky line. Jan 7, 2025
@liamappelbe liamappelbe marked this pull request as ready for review January 7, 2025 04:36
@liamappelbe liamappelbe requested a review from dcharkes January 7, 2025 04:37
@dcharkes
Copy link
Collaborator

dcharkes commented Jan 7, 2025

  • It seems that the build itself is flaky. If I change the workflow to build once and run the tests 10 times, then it's always the case that there are either 0 or 10 failures

Might we be accidentally depending on undefined behavior in native code?

@liamappelbe liamappelbe merged commit d9bb34c into main Jan 7, 2025
22 of 26 checks passed
@liamappelbe liamappelbe deleted the more_global_flake branch January 7, 2025 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcharkes dcharkes dcharkes approved these changes

Assignees
No one assigned
Labels
package:ffigen
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@liamappelbe @coveralls @dcharkes